Skip to content

Conversation

@ranj063
Copy link

@ranj063 ranj063 commented Mar 23, 2021

This pull request includes the preparatory commits paving the way for introducing Topology2.0. Existing alsatplg compiler
functionality is unmodified.

An example implementation of topology with Topology2.0 can be found here:
thesofproject/sof#3936

Introduction to Topology 2.0
-----

Topology2.0 is a high level keyword extension on top of the existing ALSA
conf topology format designed to:

1) Simplify the ALSA conf topology definitions by providing high level
   "classes" so topology designers need to write less config for common
   object definitions.

2) Allow simple reuse of objects. Define once and reuse (like M4) with
   the ability to alter objects configuration attributes from defaults.

3) Allow data type and value verification. This is not done today and
   frequently crops up in FW bug reports.

**Common Topology Classes**
-----------------------

Topology today has some common classes that are often reused throughout
with slightly altered configurations. i.e. widgets (components),
pipelines, dais and controls.

Topology2.0 introduces the high level concept of reusable "class" like
definition for a AIF_IN/AIF_OUT type object that can be used to create
topology objects.

**Common Topology Attributes**
--------------------------
Topology defines a lot of attributes per object with different types
and constraints. Today there is no easy way to validate type or
constraints and this can lead to many hard to find problems in FW at
runtime.

A new keyword "DefineAttribute" has been added to define attribute
type, size, min value, max value, enum_values. This then allows
alsatplg to validate each topology object attribute.

Topology Classes define the list of attributes that they use and
whether the attribute is mandatory, can be overridden by parent users
or is immutable. This also helps alsatplg emit the appropriate errors
for attribute misuse.

**Common Topology Arguments**
-------------------------

Arguments are used to pass essential data needed for instantiating an
object particularly needed for the object name. A new keyword
"DefineArgument" has been added to define the arguments. The order in
which the arguments are defined determines the name for the widget.
For example, in the case of the host widget, the name would be
constructed as "host.1.playback" where "1" is the pipeline_id argument
value and "playback" is the direction argument value.

**Attribute Inheritance:**
----------------------
One of the key features of Topology2.0 is howthe attribute values are
propagated from a parent object to a child object. This is accomplished
by adding attributes/arguments with the same name for a parent and an
object. By doing so, when creating a child object, the value for the
common attribute is populated from the parent. If the value is provided
in the child object instance, then it overrides the value coming from
the parent.

**ALSA Conf Parser**
----------------

All the changes being proposed and discussed here must be 100%
compliant with the ALSA conf parser. i.e. no syntax changes or
changes to semantics for any existing keyword.

It's intended that there will be NO changes to the ALSA conf parser

and all topology building changes will be in the alsatplg compiler.

@ranj063
Copy link
Author

ranj063 commented Mar 23, 2021

@lrgirdwo @kv2019i @plbossart FYI

Copy link
Member

@perexg perexg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More notes bellow...

* \return Zero on success, otherwise a negative error code
*/
int snd_tplg_add_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t);
int snd_tplg_add_element(snd_tplg_t *tplg, snd_tplg_obj_template_t *t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it's a bit late to rename the exported function. You may use 'cinstance' (class instance) or another name for the objects defined in the configuration.



static int lookup_channel(const char *c)
int lookup_channel(const char *c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of the non-static function should follow 'tplg_' prefix (see tplg_local.h).

@perexg
Copy link
Member

perexg commented Mar 24, 2021

I have a bit mixed feelings about the place for this upper level of the parser. I see an analogy to the C pre-processor. The original idea was to have a file (static topology) and an application (dynamic topology management) input. This upper level is used only for the static topology decription.

Perhaps, it may be nice to have this layer completely separate - the text file as input and generate the text file (original syntax) on output. The implementation can be moved to the alsatplg tool. If we see that we can use this code also in the dynamic way (applications), then we can reconsider to add it to the topology library.

@lgirdwood lgirdwood self-requested a review March 24, 2021 16:23
@lgirdwood
Copy link

@perexg fwiw, we would also want to use this code as a way to create alsa-plugin audio pipelines that reuse the SOF processing modules and would run on the host. i.e. we would have the same topology definition that could be used by host or DSP depending on use case. In this case it would be an advantage to have the API in alsa-lib (even if we have to make it a high level API ??)

@ranj063
Copy link
Author

ranj063 commented Mar 24, 2021

I have a bit mixed feelings about the place for this upper level of the parser. I see an analogy to the C pre-processor. The original idea was to have a file (static topology) and an application (dynamic topology management) input. This upper level is used only for the static topology decription.

Perhaps, it may be nice to have this layer completely separate - the text file as input and generate the text file (original syntax) on output. The implementation can be moved to the alsatplg tool. If we see that we can use this code also in the dynamic way (applications), then we can reconsider to add it to the topology library.

Thanks for your review @perexg. The problem with having it as a separate layer and converting it to the original syntax would make the conf files unmaintainable. The main problem that we would like to address with Topology2.0 is that the original syntax is not easy to read and process when it has the full topology definition for a machine. So I see topology2.0 as an extension to the original syntax instead of a separate layer.

@perexg
Copy link
Member

perexg commented Mar 24, 2021

Thanks for your review @perexg. The problem with having it as a separate layer and converting it to the original syntax would make the conf files unmaintainable.

I don't understand your point. The layer may be independent inside alsatplg. So from the usage POV, there's no difference. The alsatplg may just read the tplg 2.0 conf file, run the class/object evaluator, generate new v1.0 configuration file (in-memory only) and pass it to the libatopology . I don't say to store / maintain the v1.0 configuration here. It may be useful to dump it only for the debugging purposes.

@perexg
Copy link
Member

perexg commented Mar 24, 2021

@perexg fwiw, we would also want to use this code as a way to create alsa-plugin audio pipelines that reuse the SOF processing modules and would run on the host. i.e. we would have the same topology definition that could be used by host or DSP depending on use case. In this case it would be an advantage to have the API in alsa-lib (even if we have to make it a high level API ??)

I'm not against to include this to libatopology on demand. But your mentioned mechanism is not prepared (dynamic topology loading to the driver etc.). And my point is, that this higher layer can be nicely isolated from the binary topology generation. If there will be another syntax in the future, we can just make this layer optional.

@ranj063
Copy link
Author

ranj063 commented Mar 24, 2021

I don't understand your point. The layer may be independent inside alsatplg. So from the usage POV, there's no difference. The alsatplg may just read the tplg 2.0 conf file, run the class/object evaluator, generate new v1.0 configuration file (in-memory only) and pass it to the libatopology . I don't say to store / maintain the v1.0 configuration here. It may be useful to dump it only for the debugging purposes.

Ahh I see. that makes sense. I will add the class/object parser as an additional step in alsatplg. Thanks!

@lgirdwood
Copy link

@perexg fwiw, we would also want to use this code as a way to create alsa-plugin audio pipelines that reuse the SOF processing modules and would run on the host. i.e. we would have the same topology definition that could be used by host or DSP depending on use case. In this case it would be an advantage to have the API in alsa-lib (even if we have to make it a high level API ??)

I'm not against to include this to libatopology on demand. But your mentioned mechanism is not prepared (dynamic topology loading to the driver etc.). And my point is, that this higher layer can be nicely isolated from the binary topology generation. If there will be another syntax in the future, we can just make this layer optional.

Fwiw, @ranj063 has a PR pending for dynamic pipelines in the driver, but this does not yet have a external interface for userspace to push the topology binaries. The plugin based pipeline would work as follows

  1. Sound server opens a plugin based pipeline
  2. plugin loads topology conf and parses
  3. plugin creates a host (frontend) processing pipline for the audio processing parts to be performed on host.
  4. (optional) plugin sends binary topology to driver to create (backend) processing pipeline in DSP.

@ranj063
Copy link
Author

ranj063 commented Mar 30, 2021

I'm not against to include this to libatopology on demand. But your mentioned mechanism is not prepared (dynamic topology loading to the driver etc.). And my point is, that this higher layer can be nicely isolated from the binary topology generation. If there will be another syntax in the future, we can just make this layer optional.

@perexg @lgirdwood I have now updated the Topology2.0 compiler to be part of alsatplg. This PR has not been updated with the mostly function exports needed for parsing/saving topology elements.

I am still working on refactoring the alsatplg compiler but the initial version can be found here (top 31 commits)
https://github.com/ranj063/alsa-utils/commits/topic/topology2.0

@perexg
Copy link
Member

perexg commented Mar 30, 2021

I'm not against to include this to libatopology on demand. But your mentioned mechanism is not prepared (dynamic topology loading to the driver etc.). And my point is, that this higher layer can be nicely isolated from the binary topology generation. If there will be another syntax in the future, we can just make this layer optional.

@perexg @lgirdwood I have now updated the Topology2.0 compiler to be part of alsatplg. This PR has not been updated with the mostly function exports needed for parsing/saving topology elements.

I am still working on refactoring the alsatplg compiler but the initial version can be found here (top 31 commits)
https://github.com/ranj063/alsa-utils/commits/topic/topology2.0

Nice, but won't be better to not generate the text output internally - snd_tplg_save_printf(), but the snd_config_t tree ? You can save / dump it anytime using snd_config_save() function (including subtrees for debugging). It also means, that you may copy the configuration blocks directly without parsing / ASCII print sequences. This may help to integrate things to alsa-lib later, too.

EDIT: The in-place substitutions (remove classe / object trees and extend the others) in the snd_config_t tree are also allowed.

@ranj063
Copy link
Author

ranj063 commented Mar 30, 2021

Nice, but won't be better to not generate the text output internally - snd_tplg_save_printf(), but the snd_config_t tree ? You can save / dump it anytime using snd_config_save() function (including subtrees for debugging). It also mean, that you may copy the configuration blocks directly without parsing / ASCII print sequences. This may help to integrate things to alsa-lib later, too.

Indeed @perexg , I think it will be better to generate the snd_config_t tree. This will reduce the number of exports I will need. Thanks for the suggestion

ranj063 added 2 commits March 31, 2021 23:32
Create the snd_config_mak_add() and export it
for use in the Topology2.0 pre-processor in
alsatplg.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Export snd_strlcpy() for use in the Topology2.0 pre-processor
in alsatplg.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
Copy link
Author

ranj063 commented Apr 1, 2021

@perexg I've updated this PR now to generate the config tree from the 2.0 conf file. I really only need 2 exports that will be useful in the 2.0 compiler. I have the work in progress version of the compiler code here:
https://github.com/ranj063/alsa-utils/tree/topic/topology2.0-v2

And the topology examples here:
thesofproject/sof#3983

Could you please let me know if you are OK with the syntax and the direction for the compiler?

* <dt>-ENOMEM<dd>Out of memory.
* </dl>
*/
int snd_config_make_add(snd_config_t **config, char *id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be replaced with snd_config_make() + snd_config_add() calls. I don't think that we need to export another fcn for this functionality.

@perexg
Copy link
Member

perexg commented Apr 1, 2021

@perexg I've updated this PR now to generate the config tree from the 2.0 conf file. I really only need 2 exports that will be useful in the 2.0 compiler. I have the work in progress version of the compiler code here:
https://github.com/ranj063/alsa-utils/tree/topic/topology2.0-v2

And the topology examples here:
thesofproject/sof#3983

Could you please let me know if you are OK with the syntax and the direction for the compiler?

Thank you for your work. Before I start a deeper review, do you think that we can simplify the code more? I mean that the current code creates own structures / lists for the already known information which is stored in the configuration tree. I think that it may be possible to create the upper level functions which do introspection in the config tree and remove the internal structures like:

/* remove struct tplg_class / struct tplg_object etc. */

/* returns the class name */
const char *tplg_class_name(snd_config_t *class_conf);

/* class lookup */
snd_config_t *tplg_class_find_by_name(const char *name);

/* return the attribute type */
enum tplg_class_param_type tplg_class_attribute(snd_config_t *attribute_conf);

/* lookup by class and attribute name /
snd_config_t *tplg_class_attribute_find_by_name(const char *class_name, const char *attribute_name);

/* iterate through all classes  and it's attributes */
snd_config_search(top_config, "Class", &classes);
snd_config_for_each(i, next, classes) {
   _class = snd_config_iterator_entry(i);
   snd_config_search(_class, "DefineAttribute", &attributes);
   snd_config_search(i2, next2, attributes) {
      ...validate...;
   }
}

/* do objects */
snd_config_search(top_config, "Object", &objects);
snd_config_for_each(i, next, objects) {
   _class = snd_config_iterator_entry(i);
   ... validate, generate ...;
}

It's just an idea... It may remove the 'config -> C structure' conversion code, but I don't know if the config helper functions to extract the required information won't add more code than the current implementation. Also, if there are some internal states, they must be handled inside the config tree in my proposal, too.

@ranj063
Copy link
Author

ranj063 commented Apr 1, 2021

It's just an idea... It may remove the 'config -> C structure' conversion code, but I don't know if the config helper functions to extract the required information won't add more code than the current implementation. Also, if there are some internal states, they must be handled inside the config tree in my proposal, too.

Thanks for your feedback @perexg. I can certainly look into replacing the c-structs with config helper functions. Let me work on it and get back with input on which version is better in terms of the number of lines of code and readability.

@ranj063
Copy link
Author

ranj063 commented Apr 1, 2021

@perexg I was wondering if there's a way for me to use the config parser to evaluate equations? For example, for the buffer object, I'd like to compute the buffer size using its attributes.

@perexg
Copy link
Member

perexg commented Apr 1, 2021

@perexg I was wondering if there's a way for me to use the config parser to evaluate equations? For example, for the buffer object, I'd like to compute the buffer size using its attributes.

You may get any item / subtree from the config tree (snd_config_search accepts id chaining like "id1.id2.id3") and you can do own operations / extractions on top of it. But I admit, that I don't understand your question completely. An example may help.

@ranj063
Copy link
Author

ranj063 commented Apr 1, 2021

You may get any item / subtree from the config tree (snd_config_search accepts id chaining like "id1.id2.id3") and you can do own operations / extractions on top of it. But I admit, that I don't understand your question completely. An example may help.

For ex: the buffer class has an attribute called size in my class:
thesofproject/sof@8d2c375

But the size of the buffer depends on the other attributes periods/format/channels/rate etc. Is there a way for me to add a reference to the formula this attribute can use to set it value.

@perexg
Copy link
Member

perexg commented Apr 1, 2021

For ex: the buffer class has an attribute called size in my class:
thesofproject/sof@8d2c375

But the size of the buffer depends on the other attributes periods/format/channels/rate etc. Is there a way for me to add a reference to the formula this attribute can use to set it value.

There is no such mechanism. You need to write your own. You can replace the "auto" values in the tree with the results (fixed values) when all dependencies are known.

@ranj063
Copy link
Author

ranj063 commented Apr 1, 2021

There is no such mechanism. You need to write your own. You can replace the "auto" values in the tree with the results (fixed values) when all dependencies are known.

OK, thats what I am doing today so I will stick to it. Thank you!

@ranj063
Copy link
Author

ranj063 commented Apr 21, 2021

There is no such mechanism. You need to write your own. You can replace the "auto" values in the tree with the results (fixed values) when all dependencies are known.

@perexg Sorry for the delay. I was out on vacation and I'm in the process of testing the new changes. I will close this PR and open a new one in alsa-utils in a couple of days.

@ranj063 ranj063 closed this Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants